Skip to content

fix(metric-engine): make calibration serialization locale-independent - #85

Merged
Aymen (aymenfurter) merged 1 commit into
microsoft:mainfrom
JuliusGruber:fix/calibration-locale-serialization
May 30, 2026
Merged

Aymen (aymenfurter) merged 1 commit into
microsoft:mainfrom
JuliusGruber:fix/calibration-locale-serialization

Conversation

@JuliusGruber

Copy link
Copy Markdown
Contributor

Description

serializeCalibration() formats numbers with Number#toLocaleString() and no locale argument, so the thousands separator follows the host machine's default locale:

  • en-US runner → dataset: 1,234 requests
  • de-DE / de-AT (and many other locales) → dataset: 1 234 requests (a U+202F narrow no-break space)

This function emits a machine-embedded markdown comment (<!-- calibration: … -->) — a serialization artifact, not user-facing display. Locale-dependent output there is non-deterministic across developer/CI machines: it produces spurious diffs, can bake an invisible non-breaking space into committed files, and would break round-trip parsing of the block. It also makes the existing serializeCalibration unit test (which asserts dataset: 1,234 requests) fail on any non-en-US locale while passing on en-US CI.

Fix

Pin the serialization to en-US, the project's canonical format. en-US output is byte-identical (CI unaffected); all other locales become deterministic. User-facing toLocaleString() call sites (webview/UI) are intentionally left locale-adaptive.

-  lines.push(`  dataset: ${cal.datasetSize.toLocaleString()} ${cal.scope}`);
-  lines.push(`  flagged: ${cal.flaggedCount.toLocaleString()} (${cal.flaggedPct}%)`);
+  lines.push(`  dataset: ${cal.datasetSize.toLocaleString('en-US')} ${cal.scope}`);
+  lines.push(`  flagged: ${cal.flaggedCount.toLocaleString('en-US')} (${cal.flaggedPct}%)`);

Related Issues

None — found while running the test suite on a non-en-US (de-AT) machine, where serializeCalibration's test goes red.

Checklist

  • npm run check passes (typecheck + lint + spellcheck + knip + tests)
  • Changes are covered by tests (the existing serializeCalibration test now passes on every locale)
  • Documentation updated (not applicable)

🤖 Generated with Claude Code

serializeCalibration() formatted numbers with Number#toLocaleString() and
no locale argument, so the group separator followed the host machine's
default locale: "1,234" on en-US, "1 234" (U+202F narrow no-break space)
on de-DE/de-AT, etc.

This output is a machine-embedded markdown comment (<!-- calibration: ... -->),
i.e. a serialization artifact rather than user-facing display. Locale-dependent
formatting there is non-deterministic across developer/CI machines: it produces
spurious diffs, bakes an invisible non-breaking space into committed files, and
would break round-trip parsing of the block.

Pin the serialization to en-US (the project's canonical format). en-US output
is byte-identical, so CI is unaffected; all other locales become deterministic.
User-facing toLocaleString() call sites (webview/UI) are intentionally left
locale-adaptive.

Surfaced by the failing serializeCalibration unit test, which asserts "1,234"
and goes red on non-en-US locales.
@JuliusGruber

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@aymenfurter Aymen (aymenfurter) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks for your contribution

@aymenfurter
Aymen (aymenfurter) merged commit 4a893ed into microsoft:main May 30, 2026
5 checks passed
Flusspferd (JuliusGruber) pushed a commit to JuliusGruber/AI-Engineering-Coach that referenced this pull request Jun 1, 2026
Resolved conflicts:
- src/core/metric-engine.ts: resolved toward upstream. Upstream microsoft#85
  (locale-independent serialization) converges with the fork's en-US pin
  (44e9532); dropped the fork's explanatory comment so the file is now
  byte-identical to upstream, retiring that fork drift.
- package.json: kept fork-additive engines.node>=20 and @types/ws 8.18.1;
  took upstream's vscode ^1.120.0, @types/vscode 1.120.0,
  @vitest/coverage-v8 4.1.7.
- package-lock.json: regenerated via npm install --package-lock-only.

Auto-merged cleanly: parser-claude{,.test}.ts, parser-opencode{,.test}.ts
(microsoft#86 workspaceRootPath), parser-codex.test.ts, panel-request-service.ts.

Build gate: npm run build:standalone OK.
Flusspferd (JuliusGruber) pushed a commit to JuliusGruber/AI-Engineering-Coach that referenced this pull request Jun 1, 2026
- Refresh AUTO sections: staleness banner (0 behind), counts block
  (52/12/1, gap 10, no DRIFT), gap-methods list (flags calibrateRule +
  runRuleTests as not-yet-bucketed TODOs), no new upstream RPC methods.
- Record the merge outcome: microsoft#53/microsoft#67/microsoft#85/microsoft#86 merged; metric-engine.ts drift
  retired (resolved toward upstream); fork-authored drift now 2 files
  (parser-codex.test.ts, panel-request-service.ts), both upstream-it.
- Preserved the human bucket A-E narrative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants